Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/get detailed door access #265

Merged
merged 21 commits into from
Dec 12, 2023
Merged

Conversation

pontussjostedt
Copy link
Contributor

@pontussjostedt pontussjostedt commented Nov 15, 2023

Lägger till en function som tar fram alla användare, som har någon typ av individuell access. Är tänkt att användas i frontenden senare för att underlätta för syrelsen att kolla vilka som har access.

Finns ett par problem för tillfället:

  1. Känner inte att mitt testa är särskilt bra, känns som att det borde finnas ett bättre sätt
  2. Borde det jag gjort finns I users eller access, smått oklart?
  3. Jag har inte uppdaterat changeloggen :)

(behöver lite feedback :))

Anledningen till att querying behövs är att styrelsen vill veta vilka de har gett individuell access till och vill inte söka igenom alla namn en efter en.

Copy link

github-actions bot commented Nov 15, 2023

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 67.35% 1353/2009
🔴 Branches 44.49% 218/490
🔴 Functions 57.52% 241/419
🟡 Lines 67.12% 1284/1913

Test suite run failed

Failed tests: 2/238. Failed suites: 1/19.
  ● forget a user

    thrown: "Exceeded timeout of 5000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      317 | });
      318 |
    > 319 | test('forget a user', async () => {
          | ^
      320 |   const couldForget = await api.forgetUser(mockNewUser0.username); // How could you forget me?
      321 |   expect(couldForget).toBeTruthy();
      322 |

      at Object.<anonymous> (test/unit/user.api.test.ts:319:1)

  ● reset password properly

    thrown: "Exceeded timeout of 5000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      329 | });
      330 |
    > 331 | test('reset password properly', async () => {
          | ^
      332 |   await api.createUser(mockNewUser1);
      333 |   const token = await api.requestPasswordReset(mockNewUser1.username);
      334 |   const newPassword = 'Detta test skrevs på valmötet 2021';

      at Object.<anonymous> (test/unit/user.api.test.ts:331:1)

Report generated by 🧪jest coverage report action from f950190

Copy link

I have deployed this PR to feat-get-detailed-door-access-ekorre.review.esek.se 🚀

@Muncherkin
Copy link
Contributor

Fix lint and write the changelog. The tests that fail always fail so no worries there (I mean, some worries but separate issue). I'm unsure if the graph you wrote is the most useful solution. It might be more general to create an userWithIndividualAccess query that takes an optional user array as a parameter or queries all users if no user is sent as an argument. In any case, the implementation seems based and banger, do the lint and changelog fixes and consider the graph ❤️💅

@@ -197,6 +197,19 @@ describe('setting/getting access for user', () => {
await setGetTest(setAccess(accessMultipleInput), getAccess, expectedAccess);
});

//This seems kinda iffy
it('getting all access users', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be worth adding a test that adds a individualAccess and then checks that it actually is returned, that way it keeps it more contained to that specific test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean that I should just check that that one user is present?
Currently 'aa0000bb' "leaks" into the test and if our seeding data ever changes it will break the test.

So do you think it would be reasonable to remove the test checking all users and instead add 2 users; one with and one without individual access. After that check so only 1 of them is returned?


expect(accessUsers).toEqual(
expect.arrayContaining(
usernamesToCheck.map((username) => expect.objectContaining({ username })),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix the linting error

Copy link
Member

@afroborg afroborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, added some comments to look into. as @Muncherkin said, update the changelog and version. also fix the linting.

* @returns All users with individual access
*/
async getUsersWithIndividualAccess(): Promise<
(PrismaUser & { access: PrismaIndividualAccess[] })[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can update the return type to use the built in prisma types:

Prisma.PrismaUserGetPayload<{
      include: {
        access: true;
      };
    }>[]

this way you don't have to explicitly typecast it

@@ -145,6 +145,11 @@ const userResolver: Resolvers = {
return reduce(users, userReduce);
},
numberOfMembers: async (_, { noAlumni }) => api.getNumberOfMembers(noAlumni === true),
usersWithIndividualAccess: async (_: unknown, __: unknown, ctx: Context) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unknown

@afroborg
Copy link
Member

afroborg commented Dec 7, 2023

Update the version in package.json and package-lock.json to 1.6.0 as well

@afroborg afroborg mentioned this pull request Dec 7, 2023
Copy link
Member

@afroborg afroborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolve the conflicts and you are g2g

@pontussjostedt pontussjostedt merged commit 6b5e0ba into main Dec 12, 2023
6 of 7 checks passed
Copy link

I have removed this deploy now 😇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants